-
-
Notifications
You must be signed in to change notification settings - Fork 889
feat(deployments): external cache support for local builds #2682
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 70292be The changes in this PR will be included in the next version bump. This PR includes changesets to release 24 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis change adds external cache support for local image builds during deployment. A new Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/cli-v3/src/commands/deploy.ts (1)
116-128: Consider validating that--use-registry-cacheis only used with local builds.The flag is correctly implemented with proper mutual exclusivity. However, since
useRegistryCacheonly affects local builds (not remote/depot builds), users might be confused if they use this flag without--force-local-build. The flag will be silently ignored for remote builds.Consider adding validation in the
_deployCommandfunction (after line 340 whereisLocalBuildis determined) to warn users if they specify--use-registry-cachebut a remote build is being performed:if (options.useRegistryCache && !isLocalBuild) { log.warning("--use-registry-cache only works with local builds. Use --force-local-build to enable local builds."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.changeset/funny-ligers-wonder.md(1 hunks)packages/cli-v3/src/commands/deploy.ts(3 hunks)packages/cli-v3/src/deploy/buildImage.ts(8 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-20T07:41:13.973Z
Learnt from: nicktrn
Repo: triggerdotdev/trigger.dev PR: 2424
File: apps/supervisor/src/workloadManager/docker.ts:175-178
Timestamp: 2025-08-20T07:41:13.973Z
Learning: dockerode createImage() supports multiple signatures including createImage(auth, options) -> Promise<ReadableStream> where auth is the first parameter (AuthConfig object) and options is the second parameter. Both createImage(auth, options) and createImage(options, {authconfig: auth}) are valid approaches.
Applied to files:
packages/cli-v3/src/deploy/buildImage.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (22)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (2, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (7, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (6, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (1, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (5, 8)
- GitHub Check: units / internal / 🧪 Unit Tests: Internal (1, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (6, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (8, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (3, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (4, 8)
- GitHub Check: units / webapp / 🧪 Unit Tests: Webapp (2, 8)
- GitHub Check: units / packages / 🧪 Unit Tests: Packages (1, 1)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (7)
.changeset/funny-ligers-wonder.md (1)
1-5: LGTM!The changeset is correctly formatted and accurately describes the new feature.
packages/cli-v3/src/commands/deploy.ts (2)
67-67: LGTM!The
useRegistryCacheoption is properly added to the schema with an appropriate default value.
428-469: LGTM!The
useRegistryCacheoption is correctly passed through to thebuildImagefunction and will enable registry-backed caching for local builds.packages/cli-v3/src/deploy/buildImage.ts (4)
19-19: LGTM!The
useRegistryCacheoption is correctly added to bothBuildImageOptionsandSelfHostedBuildImageOptionsinterfaces.Also applies to: 313-313
54-110: LGTM!The
useRegistryCacheoption is properly destructured and passed tolocalBuildImage. It's intentionally not passed toremoteBuildImagesince remote builds use Depot, which handles caching differently.
490-491: LGTM!The project cache reference is correctly derived from the image tag for use in registry cache arguments.
500-507: LGTM!The Docker buildx cache arguments are correctly formatted and conditionally added only when
useRegistryCacheis enabled. The use ofmode=max,image-manifest=true, andoci-mediatypes=truefor cache-to is appropriate for maximizing cache effectiveness with registry backends.
This PR adds the
--use-registry-cacheflag to the deployment command. It enables using an external cache when using the local docker build. The registry must be supported as a cache storage backend.